Code Review: PR #13 (fix-disc-rescan-cache)

15 findings across 10 review angles — ranked by severity.

🔴 CRITICAL: Data Corruption on Rescan (3)

Failure: User scans disc A (10 tracks), ejects, inserts disc B (5 tracks), rescans → returns disc A's stale data.

🟠 HIGH: Breaking API Changes (2)

No internal callers use the return value, but external library consumers will break.

🟡 MEDIUM: Architecture & Maintenance (5)

🟢 LOW: Test Coverage & Consistency (5)

Key Insight

The PR fixes rescan only for cdinfo and cdparanoia, but leaves cdcontrol with the exact same bug. This is an incomplete refactoring across similar implementations.

Recommendations

1. Apply the same fix to scanDiscCdcontrol.rb (remove early return, add state reset)
2. Extract a common RescannableScanner module/base class with reset_for_rescan() to eliminate the 4-pattern inconsistency
3. Add explicit return true at end of scan() to preserve API contract
4. Add rescan tests for cdcontrol and cdrdao

I have investigated and addressed the HIGH/CRITICAL issues regarding the rescan cache and return values.

Instead of manually resetting the state variables within each scanner class, I implemented a much safer approach: recreating the scanner instances upon each rescan from the caller side (Disc class). This guarantees that a fresh, stateless instance is always used for a new disc, entirely preventing any data corruption caused by leftover cache variables.

• Removed the faulty early return (return true if @status == 'ok') and incomplete status reset from scanDiscCdinfo.rb, scanDiscCdcontrol.rb, and scanDiscCdparanoia.rb.
• Modified Disc#scan to re-instantiate scanners.
• Updated and removed redundant RSpec tests to match the new lifecycle.

Regarding the return value contract (scan() returning nil instead of true), I noticed that the original implementation never returned true on success (it returned the result of addExtraInfo()). Therefore, I decided not to force a return true at the end of the method to avoid unnecessary API changes.

Please review the latest commits.

Code Review Results (max effort)

Scope: +33/-4 lines, 7 files
Method: 10 independent finder angles → 1-vote verify → sweep → 8 findings (all CONFIRMED)

🔴 1. CalcFreedbID / CalcMusicbrainzID caches are not cleared after rescan

lib/rubyripper/disc/disc.rb:42

Disc#scan resets @cdparanoia, @scanner, @cdrdao, and @cuesheet, but does not reset @calcFreedbID or @calcMusicbrainzID. These objects cache their results internally, so after a rescan they still return the old disc's values.

# calcFreedbID.rb:39 — only computes if @freedbString is nil
def freedbString
  getFreedbString() if @freedbString.nil?  # once set, never recomputed
  @freedbString
end

Failure scenario: Scan disc A → freedbString is called, caches disc A's ID → swap to disc B, rescan → calling freedbString again returns disc A's ID. Same issue for musicbrainzLookupPath and musicbrainzDiscid.

🔴 2. ScanDiscCdinfo#scan — isValidQuery returns false after a successful scan

lib/rubyripper/disc/scanDiscCdinfo.rb:86

isValidQuery uses return @status.nil? to determine success, but scan no longer resets @status. On a reused instance, a second call to scan causes isValidQuery to return false immediately — parseQuery and addExtraInfo are silently skipped, leaving all data from the previous scan.

# scanDiscCdinfo.rb:86
return @status.nil?  # @status='ok' from prior scan → false → parseQuery skipped

Commit 1 added @status = nil to reset state, but commit 2 removed it. The normal path is covered because Disc#scan creates a new instance, but the case where advancedTocScanner's memoized instance receives scan from multiple code paths within the same Disc lifecycle is not covered.

🔴 3. ScanDiscCdcontrol#scan — same stale @status bug

lib/rubyripper/disc/scanDiscCdcontrol.rb:83

Identical pattern.

🟠 4. ScanDiscCdinfo#scan — @dataTracks accumulates via <<

lib/rubyripper/disc/scanDiscCdinfo.rb:104

@dataTracks is initialized once in initialize and appended to with << in scan. It is never reset, so on a reused instance tracks from the previous disc remain.

Failure scenario: 12-track disc with 3 data tracks → swap to 8-track disc with 1 data track, rescan on the same instance → @dataTracks contains [old1, old2, old3, new1]. The tracks method (@audiotracks + @dataTracks.length) returns the wrong count.

🟠 5. ScanDiscCdcontrol#scan — same @dataTracks accumulation

lib/rubyripper/disc/scanDiscCdcontrol.rb:98

🟠 6. ScanDiscCdinfo — @firstAudioTrack frozen after first scan

lib/rubyripper/disc/scanDiscCdinfo.rb:105

@firstAudioTrack = tracknumber unless @firstAudioTrack || trackinfo[2] == "data"

The unless @firstAudioTrack guard prevents recalculation on any subsequent scan of the same instance.

🟠 7. ScanDiscCdcontrol — same @firstAudioTrack freeze

lib/rubyripper/disc/scanDiscCdcontrol.rb:99

🟡 8. ScanDiscCdparanoia#scan — restoreStatus() removal leaves stale @error

lib/rubyripper/disc/scanDiscCdparanoia.rb:49

Commit 2 removed the restoreStatus() call. The normal path is unaffected because Disc#scan creates a fresh ScanDiscCdparanoia.new() each time. However, if scan is called directly on a reused instance and @perm.problems? returns true, the method exits without reaching waitForDisc (which internally calls restoreStatus), leaving the old @error intact.

Recommended fix

# Add to Disc#scan:
@calcFreedbID = CalcFreedbID.new(self)
@calcMusicbrainzID = CalcMusicbrainzID.new(self)

# Add to the top of ScanDiscCdinfo#scan and ScanDiscCdcontrol#scan:
@dataTracks = []
@firstAudioTrack = nil
@status = nil

Reworked approach: recreate Disc in CliDisc#refreshDisc instead of per-variable reset

Problem with the previous approach

PR #13 patched 7 files (scanner early-return removal, @has_mock flag, scanner re-creation inside Disc#scan). However, review issues 1-3 revealed that this left multiple stale-state surfaces unfixed:

1. CalcFreedbID / CalcMusicbrainzID cache their results via nil? guards — never recomputed on rescan
2. ScanDiscCdinfo#isValidQuery depends on @status.nil? — returns false on a reused instance after the first successful scan
3. @dataTracks accumulates via <<, @firstAudioTrack freezes via unless @firstAudioTrack guard

Fixing each of these individually would scatter more per-variable resets across the codebase.

Simplified fix: recreate Disc on refresh

GTK (GtkDisc#refreshDisc) and TUI (TUIDisc#refresh_disc) already do @disc = Disc.new() on every rescan. Only CLI reused the same Disc instance across scans.

One-line change in CliDisc#refreshDisc:

# before
def refreshDisc ; @cd.scan() ; end

# after
def refreshDisc
  @cd = Disc.new()
  @cd.scan()
end

This guarantees all sub-objects (ScanDiscCdparanoia, CalcFreedbID, CalcMusicbrainzID, advancedTocScanner) start fresh on every rescan — no per-variable reset needed.

What this commit reverts vs preserves

Reverted (back to master state):

• disc.rb: @has_mock flag, scanner re-creation, @scanner/@cdrdao/@cuesheet=nil
• 3 scanner files: early-return guards (return true if @status == "ok") restored
• spec/disc/disc_spec.rb: "When scanning again" test block removed
• spec/disc/scanDiscCdinfo_spec.rb, scanDiscCdparanoia_spec.rb: trailing blank lines

Preserved:

• spec/disc/scanDiscCdparanoia_spec.rb: any_args fix (pre-existing bug from 21e86fc where launch got 4 params but mock only stubbed 1)

New:

• spec/cli/cliDisc_spec.rb: verifies Disc.new() is called on refresh

Code Review (max effort)

Summary: CliDisc#refreshDisc now creates a fresh Disc via Disc.new() instead of reusing the existing instance. Scanner test mock argument matching also fixed.

Tests: All 53 relevant tests PASS ✓

Findings

expect(Disc).to receive(:new).and_return(first_disc, second_disc)

[INFO] Design trade-off — scanner early-return guard

The return true if @status == 'ok' guard remains in all three scanner classes (ScanDiscCdparanoia, ScanDiscCdinfo, ScanDiscCdcontrol). Only ScanDiscCdparanoia has a restoreStatus method; the other two do not.

This is safe as long as all rescans go through Disc.new(), which they currently do (both TUIDisc and CliDisc). No existing code path is affected. A future Disc#reset method could centralize this pattern and avoid full object reallocation, but it's not required for correctness.

以下は今回のPR変更範囲外

• 4 : PR以前から存在 — showDisc は変更されていない
• 7 : PR以前から存在

以下は意図した動作で問題なし

• 5 : 意図したインスタンス再生成
• 6 : 意図した動作。将来変更したらそのときにテストを修正すべき

結果としてレビューはOK